-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
debris
commented
Feb 22, 2018
•
edited
Loading
edited
- removed all migrations from 2016
- added migration to remove bloom group from database
util/migration/src/lib.rs
Outdated
@@ -138,7 +138,7 @@ pub trait SimpleMigration: 'static { | |||
fn version(&self) -> u32; | |||
/// Should migrate existing object to new database. | |||
/// Returns `None` if the object does not exist in new version of database. | |||
fn simple_migrate(&mut self, key: Vec<u8>, value: Vec<u8>) -> Option<(Vec<u8>, Vec<u8>)>; | |||
fn simple_migrate(&mut self, key: Vec<u8>, value: Vec<u8>, col: Option<u32>) -> Option<(Vec<u8>, Vec<u8>)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only new thing in this pr. I added it, cause it's good to know what column we are migrating
Does not compile |
pub trait SimpleMigration: 'static { | ||
/// Number of columns in database after the migration. | ||
fn columns(&self) -> Option<u32>; | ||
/// Version of database after the migration. | ||
fn version(&self) -> u32; | ||
/// Index of column which should be migrated. | ||
fn migrated_column_index(&self) -> Option<u32>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this function, the trait is completely useless, cause we don't know which column we are currently migrating
I think that migrations should be completely reworked. This pr makes a minimal effort take the most out of what we currently have. |
shoud be backported together with #7934 |
batch.insert(key, value, dest)?; | ||
} | ||
} else { | ||
batch.insert(key.into_vec(), value.into_vec(), dest)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default we are just altering the database, do we really need to overwrite the data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we are not altering, but swapping :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, since it can fail we need to keep a backup until it completes